fix: verified findings from the analytics-plan research pass (consent boundary, soul fingerprint, dispatch exit status, doc counts) - #365

BEHAVIOR CHANGE — see CONTRIBUTING: touching src/soul/* wants a discussion
issue first. this needs a maintainer decision before merge.

seed.bornOn is sha256(hostname + username): unsalted, over two low-entropy
and often-guessable strings, and stable across rebirths on the same machine.
that makes it a device fingerprint an adversary can confirm offline from a
candidate (hostname, username) pair.

dreamSoul() stringified the WHOLE seed into the birth prompt, so the
fingerprint was sent to whichever model provider is configured (Anthropic /
OpenAI / Gemini / a local endpoint) on every birth. it contributes nothing
to the dream — the personality comes from randomness + bigFive — so this
was pure unnecessary egress.

add seedForPrompt(), which drops bornOn and nothing else, and use it in the
one place the seed is serialized for the provider. writeSeed() still records
the full seed on disk, so nothing about the stored soul changes.

the behavior change: the birth prompt text is now shorter by one field, so a
NEW birth will dream a slightly different soul than it would have before.
already-born souls are untouched (birth refuses to re-run).

what this does NOT fix: GET /api/soul still serves the full seed, bornOn
included, to authenticated clients (the iOS companion reads it). narrowing
or removing that is a separate decision — noted in the type comment.

document the invariant on SoulSeed.bornOn so the next person does not
re-add it, and pin the redaction with a regression test.

two bugs in the dispatch ledger. they share DispatchEntry and the same two
files, so they land together.

1. no exit code was ever captured. launchAgent() spawned detached and only
   ever attached an "error" listener for the first 150ms, which catches
   ENOENT-class launch failures and nothing about the run. dispatch_status
   then rendered any dead pid as "✓ finished", so an agent that exited 1,
   died on a missing API key, or was OOM-killed looked exactly like a clean
   run.

   attach a "close" listener and record (exitCode, exitSignal, exitedAt).
   the listener has to go on BEFORE the 150ms launch race — an agent that
   exits inside that window emits "close" first, and a listener attached
   afterwards never fires — so the result is stashed and written once the
   ledger row exists. both orderings are covered by tests.

   BEHAVIOR CHANGE in dispatch_status output. the label is now derived from
   what was actually observed:
     ▶ running / ✓ exit 0 / ✗ exit N / ✗ killed by SIG… /
     • exited (status not captured)
   the last one is the honest case for a dispatch that outlived LISA's own
   process: because the child is detached + unref'd, the listener only fires
   while LISA is alive (true for `lisa serve` and a live REPL, false for a
   one-shot CLI invocation). we say so instead of showing a checkmark.
   "✓ finished" is gone.

2. pid reuse could make signal_agent kill an unrelated process group.
   entries are retained 24h and matched by bare pid; isAlive() was just
   kill(pid, 0), so a recycled pid answers "alive". signal_agent then sends
   SIGTERM/SIGKILL to -pid — a whole process group the user owns. that
   directly contradicts the invariant in its own header ("LISA cannot kill
   an arbitrary process").

   record a start-time fingerprint at dispatch (/proc/<pid>/stat field 22 on
   linux, `ps -o lstart=` elsewhere) and require it to match before
   reporting alive or delivering a signal. pid + start time cannot be
   reused, since a recycled pid necessarily started later. a null probe
   means "cannot tell" and is treated as a match, and entries without a
   token (older ledger files) keep the old pid-only behavior, so nothing
   silently disappears.

adds entryIsAlive(e) and moves every ledger call site onto it, including
the two in web/server.ts, so the HTTP view is guarded too. DispatchView is
deliberately unchanged — exposing exit status over /api/dispatch would
change the generated API contract and is a separate call.

…emits

`repeated_failure` was declared in SuggestionCategory but no detector ever
produced it: the five literals in detectors.ts are stuck, conflict,
cost_spike, ready, idle. its supporting state was equally inert —
AdvisorState.errorCommandCounts was written once as {} in
emptyAdvisorState() and never read or incremented, under a comment
promising a "rolling memory of (command → error count)" that never rolled.
docs/PRODUCT_REVIEW_v0.9.md already called both out as dead code.

delete both. this matters beyond tidiness: it is what makes the README fix
in the next commit correct. a maintainer told "the README lists 4 categories
and the type has 6" would naturally document all six and thereby promise a
card the product cannot produce. the honest number is five.

to stop it drifting back, declare the categories as a runtime tuple and
derive the type from it, then assert in advisor.test.ts that the declared
set and the set the detectors actually emit are equal — in both directions.
the reverse direction is the one that catches this bug class.

this had to be a runtime check: tsconfig.json excludes src/**/*.test.ts and
tsx strips types without checking them, so a type-level pin inside a test
file is never evaluated by `npm run typecheck` or `npm test`. verified by
re-adding "repeated_failure" and watching the test fail.

removing errorCommandCounts is safe for existing ~/.lisa/advisor-state.json
files: loadAdvisorState() spreads parsed JSON over emptyAdvisorState(), so a
leftover key is simply ignored.

…gories

two counts in both READMEs did not match the code.

observers. the prose said "all five observers (Claude Code, Codex,
OpenCode, Aider, GitHub PRs)" but ten ship: registry.ts imports ten observer
modules, hub.ts configures ten keys, and src/integrations/ has ten adapter
dirs. the README also contradicted itself — the directory tree 490 lines
below already listed seven of them plus an ellipsis, so this was a
copy-editing miss, not staleness.

say ten, split honestly into the five coding-agent adapters and the five
others (git, shell, takoapi, managed, pty), and add the fact the old
sentence was probably reaching for: only three are on by default
(claude-code, managed, pty) — the other seven are opt-in per integration.
the tree line now lists all ten and drops the ellipsis, so the two places
agree.

advisor categories. the tree said "(stuck / conflict / ready / idle)" — 4 of
the 5 the code emits. add cost_spike, which detectors.ts has emitted since
COST_SPIKE_TOKENS landed. repeated_failure is deliberately NOT listed: it
was dead and is deleted in the previous commit, and documenting a card the
product never produces would make the docs more wrong, not less.

prose can't be unit-tested, but the facts behind it can: hub.test.ts now
pins the roster to exactly those ten names, checks that
registerBuiltinIntegrations() really registers one observer per configured
key, and pins the three-enabled-by-default split.

the checklist told contributors to add regression tests "under `test/`".
no such directory exists, and package.json's test script is
`node --import tsx --test "src/**/*.test.ts"` — so a test written where the
template said would never be collected, and CI's `npm test` step would go
green having never run it. the worst kind of stale doc: it silently produces
a test that does not test.

CONTRIBUTING.md already says it correctly ("co-located with the source
(`src/**/*.test.ts`, run by `npm test`)"). match it, and name the failure
mode so the next person does not re-introduce it.

the promise is real and currently true — there is no analytics SDK, no
tracking pixel, no third-party script and no phone-home anywhere in the
tree. nothing enforced it. one `npm install` of a convenience wrapper, or
one snippet pasted into a layout, would turn a published privacy claim into
a false statement with nobody noticing.

add a deny-list check over src/, website/src/ and the iOS companion sources,
plus package.json's dependency names. it lives in the normal suite, so it
gates every PR through ci.yml and blocks prepublishOnly too — no CI edit
needed, and it runs locally with `npm test`.

two deliberate choices:

- the promise pages are listed BY PATH, not found by grepping for English
  phrases. the Chinese pages say "无云同步、无遥测、无任何账号" and "没有分析
  SDK", which no English keyword search would ever match, so a keyword-driven
  guard would have silently covered only half the site. listing paths also
  means renaming a promise page fails loudly instead of shrinking coverage.

- the tokens are SDK-shaped, not bare words. "segment", "heap" and
  "plausible" occur in ordinary prose and identifiers here (13, 35 and 2
  files), and "amplitude" is an audio term — a guard that cries wolf gets
  deleted. two tests pin this from both sides: one asserts the list really
  matches posthog-js / googletagmanager / @sentry, so it can never go
  vacuous, and one asserts it stays quiet on the English words.

verified by injecting a tracker script into the Chinese homepage and
confirming three of these tests fail, then reverting.

the design doc this batch of fixes came out of. it is the plan for a
local-first instrumentation system: a bounded, user-readable JSONL ledger on
the user's own disk as the only sink by default, with network upload as a
second sink that is off by default, gated on the existing consent framework,
and restricted to enums and buckets.

it is a plan, not a description of current state — none of the src/telemetry/
files it describes exist yet. no telemetry is added by this PR.

it is included here because it is the source of the verification pass that
produced the other commits: §0.1 is what established the observer count is
ten, §8.5 is what the no-telemetry CI guard implements a first slice of, and
§5.1 is what flagged bornOn as unusable as an anonymous identifier.

the plan doc was written against the tree before these fixes, and three of
its stated facts are no longer (or never were) true. left alone, the PR
would ship a design doc contradicting the code in the same PR.

- §0.1 said bornOn is "私有、从不外发" (private, never leaves the machine).
  it did leave: the whole seed was serialized into the birth prompt. now
  corrected to record what was actually true, what this PR fixed, and what
  is still open (GET /api/soul still serves the full seed). this makes §5.1's
  "never use bornOn as an identifier" red line better founded, not weaker.
- §0.1 and §3.4-Q4 said advisor has 6 categories. it has 5 —
  repeated_failure was dead and is deleted here.
- Phase 0's consent item now notes that this PR added only the route-level
  mitigation, so nobody reads the checkbox as already done: the underlying
  cross-tenant consent.json is untouched and is still a hard prerequisite
  for hanging telemetry consent off it.

the guard added in 7df0048 scanned src/, website/src/ and the iOS companion
Sources only. website/public/ was not among them, and that is the one gap
that matters most: astro copies public/ to the site root verbatim, so a
tracker dropped there ships to the very site carrying the "no telemetry"
promise without ever passing through a .astro file. verified before this
commit — a googletagmanager snippet written to website/public/analytics.js
left all 7 tests green.

scan packaging/ whole rather than just ios-companion/Sources (the mac
client, launcher and gcp-relay ship too), and add scripts/, deploy/,
contracts/ and completions/ so the guard covers the same surface the promise
was originally verified against. all six added roots are currently clean, so
this is coverage, not a fix — 111 extra files, no measurable runtime cost.

also skip build/, .build/, .swiftpm/ and DerivedData/, which appear under
packaging/ once anyone builds the native clients locally, and pin the reach
of the guard with a test asserting every SCAN_ROOTS entry still exists — a
renamed directory now fails loudly instead of silently shrinking coverage.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

the doc's header cited a source document by absolute path
(/Users/.../analytics-blueprint.md) and described its own path convention in
terms of an absolute checkout location. this repo is public, so both are
local-machine detail that means nothing to a reader and needlessly names a
directory layout. the PR body flagged the first one as a follow-up; the
second was missed.

neither reference is load-bearing: the source is not published with this
repo, and the path convention is simply "relative to the repo root".

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Same defect class as the /api/consent entry directly above it, found by
sweeping the rest of the route table for the same shape.

src/web/push.ts resolved ~/.lisa with its own private lisaHome(), outside
the per-uid homeScope in src/paths.ts, and a PushSubscription carries no
owner field. The hosted edition runs one container with one LISA_HOME
(deploy/Dockerfile) and isolates tenants purely by entering
homeScope.enterWith(homeForUid(uid)) per request — so push.json was one
shared file, and none of the five /api/push routes sat behind denyRemote,
a loopback check, or an owner check. Any signed-in tenant could GET
/api/push/list and read every other tenant's ntfy topic (which IS the
send/read secret for that channel) and APNs device token, unregister
another tenant's device, or rewrite their prefs.

Verified by running the real modules under two uid scopes: tenant B saw
tenant A's subscription and could unregister it.

The store stays machine-wide on purpose — every producer wired to
PushBridge (agent activity, idle notes, mail digest, KB brief, billing
anomalies) is a host-level concern, not a per-tenant one, so this is an
operator channel and there is no per-tenant push to preserve. What was
wrong is that tenants could reach it. So: deny the routes in cloud, and
say lisaGlobalHome() out loud in push.ts instead of re-deriving ~/.lisa,
which made a deliberate choice look like an accidental scope bypass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

isCloudDeniedRoute matches the NORMALIZED pathname, but every route in
server.ts matches the RAW req.url with startsWith/===. A dot-segment path
exploits that disagreement in one request:

  GET /api/agents/recap/%2e%2e/%2e%2e/%2e%2e?sinceMinutes=1440

normalizes to "/" — so the deny gate answers "not denied" — while still
satisfying url.startsWith("/api/agents/recap"), so the handler runs. The
same shape reaches /api/agents/steps, /api/agents/transcript,
/api/agents/pty/*/output, /api/dispatch/status (which returns raw captured
agent stdout) and /api/mail/accounts, and "//api/consent/grant" reparses
its authority so even the entry added earlier in this batch is bypassable.
%2e survives upstream proxies, so Cloud Run's frontend does not neutralize
it. Verified against the real exported function.

Teaching ~80 route checks to normalize would leave the next route to
remember, so reject non-canonical paths outright, before anything routes.
That fails closed for routes added later and costs legitimate callers
nothing: clients percent-encode, and an encoded separator that survives
normalization leaves the pathname — and therefore the deny-list decision —
unchanged, so it is left alone and the prefix still matches.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

This batch taught the ledger to record exitCode/exitSignal/exitedAt, then
never consulted them in the one predicate that decides the headline label.
entryIsAlive still asked the OS about a pid we had already watched die.

Reproduced against the real module: after recordDispatch() then
recordExit(id, 3, null), entryIsAlive() is true, statusLabel() returns
"▶ running", listLiveDispatches() still contains the entry and
findDispatch() still returns it — while the row on disk carries
exitCode: 3. GET /api/dispatch/list reported alive: true for the same row.

Second consequence, and the reason this is more than cosmetic: an entry
with no startToken falls back to bare pid identity, and that is not a rare
case — recordDispatch runs after launchAgent's 150 ms launch race, so a
fast-crashing agent is already reaped when processStartToken shells out
and the token is dropped. Those entries are retained 24 h, during which
the freed pid is available for reuse; signal_agent cancel would then
deliver SIGTERM and SIGKILL to whatever process group now owns it. The
recorded exit already proved the entry was dead.

Gate on exitedAt rather than exitCode — a signal death legitimately stores
exitCode: null.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The three privacy tests added in this batch all assert on seedForPrompt's
own output — one of them re-derives the payload with
JSON.stringify(seedForPrompt(seed), null, 2) under a comment calling it
"the actual wire shape". Nothing enforced that coupling: reverting
birth.ts back to JSON.stringify(seed, null, 2) left the suite at 9 pass /
0 fail. The headline privacy fix of this PR could be silently undone.

dreamSoul is where the prompt is actually assembled and it already takes
its provider as a parameter, so export it and hand it a fake that captures
what gets sent, then assert the fingerprint is absent and that the fields
the dream does need still travel. Reverting the fix now fails this test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>